[RNE Rewrite] feat: coded, worklet-safe error handling - #1354
Conversation
barhanc
left a comment
There was a problem hiding this comment.
Only top level general comments for now as I want to first discuss the overall premises of the implementation before diving into details.
|
Agreed on all changes; I'll implement them after the weekend ;) |
Proposal for the "Error Handling in TS" item of #1208. Keeps main's machine-readable `code` contract, adopts the rewrite's message discipline, and makes both survive the worklet boundary. - errors.config.ts: 46 codes -> 13. ExecuTorch's codes are no longer mirrored into the RNE enum; they travel in a separate `etCode` field so the two code spaces stay independent. Drops `Ok`, the UnknownError/Internal overlap, and the codes the rewrite's design made unreachable (`ModuleNotLoaded` has no analogue now that a task cannot exist unloaded). - RnExecutorchError sets `name`, keeps `instanceof` working through subclasses via `new.target`, and no longer shadows the native `cause`. - Worklets cannot carry class identity across runtimes, so `rnExecutorchError()` throws the plain-data form inside them and `wrapAsync` rebuilds a real error on the RN runtime. `isRnExecutorchError()` works in both, and is the recommended check. - Native: `CodedError` + `guarded()` at every host function, so a code raised deep in the native stack reaches the app's catch block. - Rewires `yarn codegen:errors` and the CI drift check, which were orphaned on this branch.
Follow-up to the error-handling proposal: closes the two gaps left open and updates the example apps. Native (items 1 + 2) - `error::guarded(...)` now wraps all 31 host function registrations, not just model.cpp's 3. 127 remaining `jsi::JSError` / `std::runtime_error` / `std::invalid_argument` throws across 13 files became `CodedError`. Nothing but `throwJs` raises an uncoded exception any more. - This also closes the sync-worklet hole: `guard` attaches `code` to the JS Error it builds, so a VisionCamera frame processor calling a task worklet directly now receives coded errors too. Previously only the `wrapAsync` path had a fallback, and it could only ever say `Internal`. - Renames `ModelBusy`/`ModelDisposed` to `ResourceBusy`/`ResourceDisposed`. Tensors and tokenizers have exactly the same two states as models, and they are where most of these throws live. Still 13 codes. Demo apps (item 4) - Adds `describeError` / `isBusyError` / `isDisposedError` per app, replacing five different ad-hoc idioms (`e.message || String(e)`, `String(loadError)`, `e?.message ?? String(e)`, ...) across 38 sites. - text-embeddings matched disposal with `/disposed/i.test(msg)` against the error *text*. Now a `ResourceDisposed` code check. - classification drops `ResourceBusy` instead of flashing an error, on both `classify` and `classifyWorklet` — the same code arrives over either boundary. `jsi::JSError` still passes through `guard` untouched, so an error thrown by an app's own callback (e.g. inside `tensor.through(fn)`) is never rewritten with our name and code.
- utils.cpp: the error include and using-declarations were sitting inside the `#elif defined(__APPLE__)` branch, so `CodedError` and `ErrorCode` were undeclared on every non-Apple target. Moved to file scope. This is why the macOS-only local syntax check did not catch it. - model.cpp, tensor.cpp: drop namespace aliases that misc-unused-alias-decls flagged. Inside rnexecutorch::core::*, unqualified `error` already resolves to the sibling rnexecutorch::core::error, and model.cpp's `jsi` alias lost its last user when the local unwrap templates were replaced by error::unwrapEt. The alias is kept in extensions::*, where sibling lookup does not reach it.
skills-maintenance requires skills to be updated in the same change that shifts an idiom. This was missed: add-native-extension still taught `throw jsi::JSError(rt, ...)` and a bare `createFromHostFunction(..., fnBody)`, which are now exactly the two patterns the convention forbids. - New .agents/skills/error-handling/SKILL.md: the two throw forms and why they differ (worklet runtimes cannot carry class identity), catching via isRnExecutorchError, the 13-code table, the test to apply before adding a code, the C++ CodedError/guarded rules, and the app describeError helpers. - add-native-extension: template now throws CodedError and registers through error::guarded, including the namespace-alias rule (needed in extensions::*, dead in core::*) and the file-scope include placement that broke Android. - add-task-pipeline, model-schema-validation: throw-with-a-code guidance plus checklist items. - verify-and-build: `yarn codegen:errors` step, and the two verification traps this branch hit (Homebrew clang-tidy reports checks CI lacks; a macOS-only syntax check cannot see platform-conditional code). - core-guidelines and README: index the new skill.
- conversions.h: drop the redundant `core::` qualification. The file is in rnexecutorch::core::conversions, so unqualified `error` already resolves to the sibling rnexecutorch::core::error. - error.h: mark both CodedError constructors explicit. - error.h: use std::format for the unwrapEt context prefix instead of string concatenation, matching the rest of the codebase.
Addresses the review threads on #1354. Codes are no longer generated (barhanc): `scripts/errors.config.ts` and `generate-errors.ts` are deleted along with the codegen script and CI drift check. `src/core/error.ts` is now the source of truth and `cpp/core/error.h` mirrors it by hand, like every other part of the TS/JSI interface. TypeScript: - Codes are a string union, not a numeric enum, and the set shrinks from 13 to 10. A code now has to justify a distinct recovery path; the tokenizer and not-supported codes folded into the general categories. - One `RnExecuTorchError(code, message)` factory replaces the class/worklet-helper pair. It is a function, so it works unchanged on both runtimes rather than needing two spellings. - `isRnExecuTorchError(err, code?)` takes an optional code to narrow. - The internal fetcher `AbortError` class becomes a DOWNLOAD_ABORTED error; `useResourceDownload` matches the code instead of the class. C++: - `CodedError` -> `RnExecuTorchException`, `ErrorCode` -> `RnExecuTorchErrorCode`, `etCode` -> `etRuntimeErrorCode`. - `throwJs` and `makeJsError` are replaced by a single `throwJsiRnExecuTorchError(rt, e)` that only takes the C++ exception, so there is no way to reach JS without constructing one first. - `unwrapEt` leaves the error namespace and goes back to being a file-local `unwrap` helper with a single signature, no jsi::Runtime. Example apps are reverted to their original state: they are a testing ground and should surface raw errors (barhanc, #1288). The new Supertonic TTS throw sites from #1317 are converted to the convention. Agent skills are updated to match.
e1c3ae0 to
6afb23a
Compare
barhanc
left a comment
There was a problem hiding this comment.
Looks good. Some header files have outdated docs regarding what they throw, so this needs to be fixed (e.g. tensor_helpers.h). Other than that one additional comment regarding C++ error syntax sugar. I've tested the error handling on a simple example screen. It's on branch @bh/rewrite-error-handling-test if you'd need it.
There was a problem hiding this comment.
Looks good. The call sites are a bit long though, so I was thinking about maybe adding factory methods so we could do RnExecuTorchException::InvalidArgument(msg, etCode) (also I'm thinking whether RnExecuTorchException or ...Error is better) or even error::InvalidArgument(msg, etCode). Doing this cleanly would require some x-macro magic though I'm afraid, so I leave it to you to decide if it's worth it or should we stay with explicit callsites. The x-macro magic in question:
// NOLINTNEXTLINE(cppcoreguidelines-macro-usage): X-macro is used for concise error code enum, string mapping, and factory method definitions
#define FOR_ALL_RNEXECUTORCH_ERROR_CODES(V) \
V(LoadFailed, "LOAD_FAILED") \
V(ExecutionFailed, "EXECUTION_FAILED") \
V(SchemaMismatch, "SCHEMA_MISMATCH") \
V(InvalidArgument, "INVALID_ARGUMENT") \
V(InvalidState, "INVALID_STATE") \
V(ResourceDisposed, "RESOURCE_DISPOSED") \
V(ResourceBusy, "RESOURCE_BUSY") \
V(DownloadFailed, "DOWNLOAD_FAILED") \
V(DownloadAborted, "DOWNLOAD_ABORTED") \
V(Unknown, "UNKNOWN")
enum class RnExecuTorchErrorCode {
// NOLINTNEXTLINE(cppcoreguidelines-macro-usage): helper macro for X-macro expansion
#define DEFINE_ENUM(name, str) name,
FOR_ALL_RNEXECUTORCH_ERROR_CODES(DEFINE_ENUM)
#undef DEFINE_ENUM
};
constexpr const char *errorCodeToString(RnExecuTorchErrorCode code) {
switch (code) {
// NOLINTNEXTLINE(cppcoreguidelines-macro-usage): helper macro for X-macro expansion
#define DEFINE_CASE(name, str) \
case RnExecuTorchErrorCode::name: \
return str;
FOR_ALL_RNEXECUTORCH_ERROR_CODES(DEFINE_CASE)
#undef DEFINE_CASE
}
return "UNKNOWN";
}
// Alternative 1: Static Factory Methods on Exception
class RnExecuTorchException : public std::runtime_error {
// ...
// NOLINTNEXTLINE(cppcoreguidelines-macro-usage): helper macro for X-macro expansion
#define DEFINE_FACTORY(name, str) \
static RnExecuTorchException name(const std::string &message, \
std::optional<executorch::runtime::Error> etError = std::nullopt) { \
return etError ? RnExecuTorchException(RnExecuTorchErrorCode::name, message, *etError) \
: RnExecuTorchException(RnExecuTorchErrorCode::name, message); \
}
FOR_ALL_RNEXECUTORCH_ERROR_CODES(DEFINE_FACTORY)
#undef DEFINE_FACTORY
// ...
}
// Alternative 2: Namespace-Level Helper Functions
// NOLINTNEXTLINE(cppcoreguidelines-macro-usage): helper macro for X-macro expansion
#define DEFINE_NAMESPACE_FACTORY(name, str) \
inline RnExecuTorchException name(const std::string &message, \
std::optional<executorch::runtime::Error> etError = std::nullopt) { \
return etError ? RnExecuTorchException(RnExecuTorchErrorCode::name, message, *etError) \
: RnExecuTorchException(RnExecuTorchErrorCode::name, message); \
}
FOR_ALL_RNEXECUTORCH_ERROR_CODES(DEFINE_NAMESPACE_FACTORY)
#undef DEFINE_NAMESPACE_FACTORY
Description
This branch had no error module: everything threw a plain
Errorwith a message, andwrapAsyncflattened anything down toe?.message.mainhas a code-based contract, but that contract cannot survive in worklets, etc.This keeps the machine-readable
codecontract, adopts this branch's message discipline, and makes both survive worklets and JSI.etCodefield so RNE and ET errors stay independent. DropsOk, theUnknownError/Internaloverlap, andModuleNotLoaded(not applicable anymore).RnExecutorchErrorsetsname, keepsinstanceofworking through subclasses vianew.target, nocauseanymore.rnExecutorchError()throws a plain data inside them andwrapAsyncrebuilds the error.isRnExecutorchError()works in both and is the recommended check.CodedErrorplusguarded()on all host functions, and rawjsi::JSError/std::runtime_error/std::invalid_argumentthrows converted, so a code raised deep in the native stack reaches the app's catch block. This also covers the sync worklet path used by VisionCamera frame processors, which previously had no normalization at all.Introduces a breaking change?
Type of change
Tested on
Testing instructions
yarn install && yarn workspace react-native-executorch prepareyarn typecheck && yarn lint, both clean.yarn codegen:errorsthengit diff --exit-codeonsrc/errors/codes.tsandcpp/core/error_codes.h, no output.Screenshots
Related issues
#1208
Checklist
Additional notes
jsi::JSErrordeliberately passes throughguarduntouched, so an error thrown by an app's own callback is never rewritten with our name and code.